Skip to content

fix: reuse only user-supplied values when no values flag is given (#464) - #1087

Open
rishabhvenu wants to merge 1 commit into
databus23:masterfrom
rishabhvenu:fix/default-reuse-user-values
Open

rishabhvenu wants to merge 1 commit into
databus23:masterfrom
rishabhvenu:fix/default-reuse-user-values

Conversation

@rishabhvenu

Copy link
Copy Markdown
Contributor

Fixes #464

helm diff upgrade without any --set/--values flag does not show changes made to the chart's own values.yaml. For a release installed from a local chart, editing a value in values.yaml gives an empty diff, while helm upgrade does apply the change.

Cause

When no value flag is given, template() in cmd/helm.go fetches the values of the release with helm get values --all and passes them to helm template as --values. With --all that output is the fully coalesced values, so it contains the whole values.yaml of the chart the release was installed from. A --values file takes precedence over the values.yaml of the new chart, so every changed default is overridden by its old value. (The root cause was worked out by @antoineozenne in the issue.)

This is what helm upgrade --reuse-values does, not what helm upgrade does by default. In reuseValues() the default case is only:

if len(newVals) == 0 && len(current.Config) > 0 {
    newVals = current.Config
}

current.Config holds the user-supplied values. The chart's values are left alone.

Fix

Without --reuse-values, fetch the values without --all, which is current.Config. If the release has no user-supplied values, helm get values prints null, and no --values is passed at all. --reuse-values still uses --all, and --reset-then-reuse-values already fetched without --all. The same code runs with HELM_DIFF_USE_UPGRADE_DRY_RUN=true, so that mode is fixed too.

Behaviour change

The default diff now includes changes to the chart's default values. A diff that used to be empty can now show changes, and --detailed-exitcode then returns 2 for it. Those are the changes helm upgrade makes. User-supplied values of the release are still kept, as before.

Tests

  • TestUpgradeCommand_Execution_ReuseValues: the case "existing release without value flags" from fix: skip --reuse-values for a release that is not installed (#481) #1086 expected helm get values --all. It now expects the call without --all, since that expectation was this bug. Two new cases cover HELM_DIFF_USE_UPGRADE_DRY_RUN and a release without user-supplied values, where helm get values prints null and no --values may be passed. All cases now also check whether --values reaches the render command. The three default-path cases fail on master. The fake helm can print a separate output for helm get values through HELM_DIFF_FAKE_VALUES_OUTPUT.
  • I ran the built binary as a plugin with Helm v3.17.3 and v4.3.0 against a local kube-apiserver and etcd (no kubelet). The chart has one ConfigMap. I installed one release without values and one with --set user=from-install, then changed a default in values.yaml and added a new key. For each row I compared helm diff upgrade with helm upgrade --dry-run=server:
flags plain Helm master this PR
none new default, user value kept old default same as Helm
none, HELM_DIFF_USE_UPGRADE_DRY_RUN=true new default, user value kept old default same as Helm
--reset-values new default, user value dropped same as Helm unchanged
--reset-then-reuse-values new default, user value kept same as Helm unchanged
-f extra.yaml new default, user value dropped same as Helm unchanged
--reuse-values old default, user value kept old default, user value kept unchanged

Both Helm versions give the same results. Only the two rows without flags change. With only an existing value edited, helm diff upgrade --allow-unreleased prints an empty diff on master and the changed line with this PR.

  • make format, make lint and make test pass, and golangci-lint v2.14.0 reports 0 issues.

Not tested: a kind cluster, so the CI integration job has not been run locally. I did not add a scripts/issues/464.sh; I can add one if you want it.

Not part of this PR

  • Diff is incorrect when values in values.yaml change and --reuse-values is used #457 is not fixed. With --reuse-values, a key that is new in the chart's values.yaml still appears in the diff, because a --values file can only be merged over the new chart's values and Helm replaces them.
  • With HELM_DIFF_USE_UPGRADE_DRY_RUN=true, --reset-values is not passed on to helm upgrade, so a release with user-supplied values keeps them in the diff. That is the same before and after this change.

…tabus23#464)

Without any --set/--values flag, helm-diff fetched the values of the
release with `helm get values --all` and passed them to the render
command as --values. That output contains the whole values.yaml of the
chart the release was installed from, and a --values file takes
precedence over the values.yaml of the new chart. Changes to the
chart's own values.yaml were therefore missing from the diff.

`helm upgrade` only does that with --reuse-values. By default it reuses
just the user-supplied values of the release. Fetch the values without
--all in that case, and pass no --values at all when the release has no
user-supplied values (`helm get values` prints "null" then).

The diff for --reuse-values and --reset-then-reuse-values does not change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Diff is incorrect when values in values.yaml change and using local Chart

1 participant